-
Notifications
You must be signed in to change notification settings - Fork 8.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[ML] Extend MlUrlGenerator to support other ML pages #75696
Conversation
Pinging @elastic/ml-ui (:ml) |
Could we expose
Naming nit: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
app arch updates LGTM
Removed the hash here 64b936b
Thanks for bringing it up. I will make the changes for these in a follow up PR as it involves reviews from another team. |
pause: boolean; | ||
value: number; | ||
} | ||
|
||
export interface TimeRange { | ||
from: string; | ||
to: string; | ||
mode?: 'absolute' | 'relative'; | ||
mode?: 'absolute' | 'relative' | 'quick'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@qn895 I'm slightly confused on how both these fields get used.
I don't even see any place in the code using them (nor here or in the data plugin), to the point I would consider removing mode
🤨
I'd love to sync on this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mlUrlGeneratorState as DataFrameAnalyticsExplorationUrlState | ||
); | ||
case ML_PAGES.DATA_VISUALIZER: | ||
return this.createDataVisualizerUrl(mlUrlGeneratorState as DataVisualizerUrlState); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these 3 are the same and so cold fall through to one case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated here 4d6a5a0
/** | ||
* Creates URL to the Anomaly Explorer page | ||
*/ | ||
private createExplorerUrl({ | ||
refreshInterval, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this file is now very large. would it be possible/convenient to break it into multiple files? one per page perhaps.
It might make it easier to manage in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refactor to separate files here 4d6a5a0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Latest changes LGTM! 👍 But please pay attention to the MLPageState
suggestion
CLASSIFICATION: 'classification', | ||
} as const; | ||
|
||
type DATAFRAME_ANALYTICS_TYPE = typeof ANALYSIS_CONFIG_TYPE[keyof typeof ANALYSIS_CONFIG_TYPE]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we only use this notation for constants. For types, it's a camel case starting with capital.
type DATAFRAME_ANALYTICS_TYPE = typeof ANALYSIS_CONFIG_TYPE[keyof typeof ANALYSIS_CONFIG_TYPE]; | |
type DataFrameAnalyticsType = typeof ANALYSIS_CONFIG_TYPE[keyof typeof ANALYSIS_CONFIG_TYPE]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated here 8248d7f
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Latest changes LGTM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few extra minor function / file renames for the data frame analytics pages are needed.
x-pack/plugins/ml/public/ml_url_generator/dataframe_analytics_urls_generator.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/ml/public/ml_url_generator/dataframe_analytics_urls_generator.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/ml/public/ml_url_generator/dataframe_analytics_urls_generator.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Latest edits LGTM!
💚 Build SucceededBuild metrics@kbn/optimizer bundle module count
async chunks size
page load bundle size
distributable file count
History
To update your PR or re-run it, just comment with: |
Co-authored-by: Dima Arnautov <dmitrii.arnautov@elastic.co>
Summary
This PR is part of #69265 to extend MlUrlGenerator for other apps within Kibana to access and to convert ML urls to non-hash paths.
Example usage:
Or preserve search within
ml
pluginAddition parameters that belong to globalApp
_g
and appState_a
or custom to the page itself are also supportedChecklist